-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feat] EventUserArgumentResolver 적용 및 FE 요청에 따른 추가 API 구현 (#35) #38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
특이사항 및 생각은 코멘트에 정리했습니다!
build.gradle
Outdated
// jackson | ||
implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310' | ||
implementation 'com.fasterxml.jackson.core:jackson-databind' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseFcfsInfoDto에 존재하는 DateTime 타입 필드의 직렬화를 위해 jackson 라이브러리를 추가하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jackson이 스프링부트에 내장되어 있어서 필요 없을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀주신 부분 반영했습니다:)
// 유효하지 않은 요청은 정적 리소스로 간주하여 ResourceHttpRequestHandler가 대신 처리하기에, HandlerMethod가 아닌 경우는 무시 | ||
if (!(handler instanceof HandlerMethod)) { | ||
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
404가 발생해야 하는데 요청을 가로채는 과정에서 500이 뜨던 버그가 있어 수정했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
내부 경로가 아니면 HandlerMethod가 아닌 handler이 올 수도 있다는 것을 새롭게 알게 되었네요. 기존에 swagger-ui 경로에 접근하지 못하던 문제도 이걸로 처리할 수 있었겠네요
@@ -22,6 +23,7 @@ | |||
|
|||
@Slf4j | |||
@SpringBootTest | |||
@TestPropertySource(locations = "classpath:application-test.yml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실 DB에 접근하는 부하 테스트의 경우 test.yml을 통해 h2를 활용하도록 수정했습니다.
when(authInterceptor.preHandle(any(), any(), any())).thenReturn(true); | ||
when(eventUserArgumentResolver.supportsParameter(any())).thenReturn(true); | ||
when(eventUserArgumentResolver.resolveArgument(any(), any(), any(), any())) | ||
.thenReturn(mockUserInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실제 테스트 코드에서도 항상 호출되는 코드이기에 mocking을 통해 테스트가 가능하도록 설정했습니다.
return String.format(ConstantUtil.AUTH_CODE_CREATE_REGEX, new Random().nextInt(1000000)); | ||
StringBuilder authCode = new StringBuilder(); | ||
Random random = new Random(); | ||
for(int i=0; i<ConstantUtil.AUTH_CODE_LENGTH; i++){ | ||
authCode.append(random.nextInt(10)); | ||
} | ||
return authCode.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전 코드가 가독성이 떨어진다고 판단하여 명시적으로 특정 길이의 인증번호를 추출하는 것으로 표현했습니다.
Jws<Claims> claims = (Jws<Claims>) request.getAttribute(JWTConst.Token); | ||
String userId = claims.getPayload().get(ConstantUtil.CLAIMS_USER_KEY).toString(); | ||
String role = claims.getPayload().get(ConstantUtil.CLAIMS_ROLE_KEY).toString(); | ||
return new EventUserInfo(userId, role); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RequiredArgsConstructor | ||
@Configuration | ||
public class WebConfig implements WebMvcConfigurer { | ||
@Autowired | ||
private AuthInterceptor authInterceptor; | ||
|
||
@Autowired | ||
private AdminArgumentResolver adminArgumentResolver; | ||
private final AuthInterceptor authInterceptor; | ||
private final AdminArgumentResolver adminArgumentResolver; | ||
private final EventUserArgumentResolver eventUserArgumentResolver; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필드 주입이 3개 이상이라 생성자 주입으로 바꾸었습니다.
public static final String CLAIMS_USER_KEY = "userId"; | ||
public static final String CLAIMS_ROLE_KEY = "role"; | ||
public static final String CLAIMS_USER_NAME_KEY = "userName"; | ||
public static final String JWT_USER_KEY = "eventUser"; | ||
public static final String CHARACTERS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; | ||
public static final String LOCATION = "Location"; | ||
|
||
public static final double LIMIT_NEGATIVE_CONFIDENCE = 99.5; | ||
public static final int COMMENTS_SIZE = 20; | ||
public static final int SCHEDULED_TIME = 1000 * 60 * 60 * 2; | ||
public static final int SHORT_URL_LENGTH = 10; | ||
public static final int USER_ID_LENGTH = 8; | ||
public static final int AUTH_CODE_LENGTH = 6; | ||
public static final int JWT_LIFESPAN = 1; // 1시간 | ||
public static final int AUTH_CODE_EXPIRE_TIME = 5; // 5분 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파편화된 Key 문자열과 매직넘버를 상수로 관리하고자 했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 것 같습니다. 저도 여기에다 admin 부분을 등록해서 변경해둬야겠네요
if(nowDateTime.isBefore(eventStartTime) && nowDateTime.plusHours(3).isAfter(eventStartTime)) { | ||
return new ResponseFcfsInfoDto(nowDateTime, "countdown"); | ||
} else if(eventStartTime.isBefore(nowDateTime) && eventStartTime.plusHours(7).isAfter(nowDateTime)) { | ||
return new ResponseFcfsInfoDto(nowDateTime, "progress"); | ||
} else { | ||
return new ResponseFcfsInfoDto(nowDateTime, "waiting"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FE에서 요구한 대로 이벤트에 대한 정보를 추출하는 메서드를 작성했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3, 7이라는 숫자는 추후에 변경될 수도 있으니 별도로 분리해두는 것도 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀주신 부분까지 반영해서 머지하겠습니다:)
stringRedisTemplate.opsForZSet().add(FcfsUtil.winnerFormatting(eventSequence.toString()), userId, System.currentTimeMillis()); | ||
stringRedisTemplate.opsForSet().add(FcfsUtil.participantFormatting(eventSequence.toString()), userId); | ||
log.info("Event Sequence: {}, User ID: {}, Timestamp: {}", eventSequence, userId, timestamp); | ||
return true; | ||
} | ||
|
||
public boolean isParticipated(Long eventSequence, String userId) { | ||
return Boolean.TRUE.equals(stringRedisTemplate.opsForSet().isMember(FcfsUtil.participantFormatting(eventSequence.toString()), userId)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"당첨 여부"와 별개로 유저가 정답을 맞혔는지에 대한 "참여 여부"를 판정해야 한다는 요구사항을 반영하기 위해 Set을 추가했고, 중복 참여 여부를 당첨 여부로 판정했던 기존 로직을 교체했습니다.
추후에 선착순 이벤트에서 요청 시각 기반으로 "순서"를 고려해야 할 가능성이 존재하는 "당첨 여부"와 달리,
"참여 여부"는 순서를 고려할 필요가 없고 자정이 되면 휘발성으로 소멸하기 때문에 SortedSet이 아닌 단순 Set을 사용했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
build.gradle
Outdated
// jackson | ||
implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310' | ||
implementation 'com.fasterxml.jackson.core:jackson-databind' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jackson이 스프링부트에 내장되어 있어서 필요 없을 것 같아요!
public static final String CLAIMS_USER_KEY = "userId"; | ||
public static final String CLAIMS_ROLE_KEY = "role"; | ||
public static final String CLAIMS_USER_NAME_KEY = "userName"; | ||
public static final String JWT_USER_KEY = "eventUser"; | ||
public static final String CHARACTERS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; | ||
public static final String LOCATION = "Location"; | ||
|
||
public static final double LIMIT_NEGATIVE_CONFIDENCE = 99.5; | ||
public static final int COMMENTS_SIZE = 20; | ||
public static final int SCHEDULED_TIME = 1000 * 60 * 60 * 2; | ||
public static final int SHORT_URL_LENGTH = 10; | ||
public static final int USER_ID_LENGTH = 8; | ||
public static final int AUTH_CODE_LENGTH = 6; | ||
public static final int JWT_LIFESPAN = 1; // 1시간 | ||
public static final int AUTH_CODE_EXPIRE_TIME = 5; // 5분 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 것 같습니다. 저도 여기에다 admin 부분을 등록해서 변경해둬야겠네요
// 유효하지 않은 요청은 정적 리소스로 간주하여 ResourceHttpRequestHandler가 대신 처리하기에, HandlerMethod가 아닌 경우는 무시 | ||
if (!(handler instanceof HandlerMethod)) { | ||
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
내부 경로가 아니면 HandlerMethod가 아닌 handler이 올 수도 있다는 것을 새롭게 알게 되었네요. 기존에 swagger-ui 경로에 접근하지 못하던 문제도 이걸로 처리할 수 있었겠네요
if(nowDateTime.isBefore(eventStartTime) && nowDateTime.plusHours(3).isAfter(eventStartTime)) { | ||
return new ResponseFcfsInfoDto(nowDateTime, "countdown"); | ||
} else if(eventStartTime.isBefore(nowDateTime) && eventStartTime.plusHours(7).isAfter(nowDateTime)) { | ||
return new ResponseFcfsInfoDto(nowDateTime, "progress"); | ||
} else { | ||
return new ResponseFcfsInfoDto(nowDateTime, "waiting"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3, 7이라는 숫자는 추후에 변경될 수도 있으니 별도로 분리해두는 것도 좋을 것 같습니다.
#️⃣ 연관 이슈
📝 작업 내용
참고 이미지 및 자료
💬 리뷰 요구사항